Skip to content

[FL-729] [POC] Dimensioned output #992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[FL-729] [POC] Dimensioned output #992

wants to merge 4 commits into from

Conversation

benflexcompute
Copy link
Collaborator

@benflexcompute benflexcompute commented May 5, 2025 •
edited
Loading

⚠️ Main issue worth thinking:

  1. Currently, the output unit selection is done by checking the unit system used in the simulation.json. Is this straightforward to user? It is almost impossible for user to change this without resetting the unit system. Should we store the unit system per output field?
    For e.g.
class SolverVariable(Variable):
    ...
    unit_system_name: str = pd.Field("SI")
    
    def in_SI(self) -> SolverVariable:
        return SolverVariable(unit_system_name = "SI", **(self.model_dump()))

and then allow per-instance overload?

  1. There will be overlapping with the base branch. For example the varaible name in the UDF source code. (SolverVaraible -> solver_side_name) I think it is fine we do not use solver_side_name but instead just the user facing name but I was thinking to retire user-facing variable string completly so the solver does not need to know the mapping from "solverNavierStokes" --> "NavierStokes_solver".

👀 Following script demonstrate usage.

import json

import flow360 as fl
from flow360.component.simulation.framework.param_utils import AssetCache
from flow360.component.simulation.solver_builtins import *

# from flow360.component.simulation.primitives import Surface
from flow360.component.simulation.unit_system import (
    MassFluxType,
)


print(">> ", fl.SI_unit_system[MassFluxType.dim_name])
print(">> ", MassFluxType.dim)

with imperial_unit_system:
    params = fl.SimulationParams(
        operating_condition=fl.AerospaceCondition(velocity_magnitude=10),
        outputs=[fl.VolumeOutput(output_fields=[solutionNavierStokes, solutionNavierStokes])],
        private_attribute_asset_cache=AssetCache(project_length_unit="m"),
    )
with open("ttt.json", "w") as f:
    f.write(params.model_dump_json())

with open("ttt.json", "r") as f:
    param_as_dict = json.load(f)

params, _validation_errors, _ = fl.services.validate_model(
    params_as_dict=param_as_dict,
    validated_by=fl.services.ValidationCalledBy.PIPELINE,
    root_item_type="VolumeMesh",
    validation_level="CASE",  # Only need to validate CASE related fields.
)
print(">> ", _validation_errors)

translated, _ = fl.services.simulation_to_case_json(params, "cm")
with open("translated.json", "w") as f:
    json.dump(translated, fp=f, indent=4)
"userDefinedFields": [
        {
            "expression": "solutionNavierStokes_SI[0] = NavierStokes_solution[0] / 0.8163265306122448; solutionNavierStokes_SI[1] = NavierStokes_solution[1] / 0.0023988860123275884; solutionNavierStokes_SI[2] = NavierStokes_solution[2] / 0.0023988860123275884; solutionNavierStokes_SI[3] = NavierStokes_solution[3] / 0.0023988860123275884; solutionNavierStokes_SI[4] = NavierStokes_solution[4] / 7.049451272672674e-06;",
            "name": "solutionNavierStokes_SI"
        }
    ],

❓ TODO:

  • Proper unit test.
  • Figure out a proper way to associate the source entity with the variable.

benflexcompute
benflexcompute commented May 5, 2025
View reviewed changes
flow360/__init__.py
@@ -134,6 +134,52 @@
Transformation,
)
from flow360.component.simulation.simulation_params import SimulationParams

# from flow360.component.simulation.solver_builtins import (
# CD,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These won't work now.

@andrzej-krupka
Copy link
Contributor

andrzej-krupka commented May 6, 2025 •
edited
Loading

@benflexcompute I like the idea of retiring the user-facing names, but I feel we miscommunicated overall. I see a couple big issues with this approach:

  1. We disregard the features offered by blueprint / Expression class altogether by forcing the user into using a single SolverVariable only
  2. This probably isn't compatible with the WebUI format of specifying user expressions
  3. We need to be strategic about merging as this will likely conflict with Reorganized solver variables into target namespaces #986 and Nested expression support + expression validation endpoints #946

Can we have a call to discuss it a bit more?

andrzej-krupka
andrzej-krupka reviewed May 6, 2025
View reviewed changes
flow360/component/simulation/translator/solver_translator.py
"""Convert output field to string"""
if isinstance(output_field, str):
return output_field
elif isinstance(output_field, SolverVariable):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand this. The output field will usually not be a raw SolverVariable, but rather an expression.

If the user writes:

fl.timeStepSize * 2

the resulting object type is an Expression. If he assigns a raw variable to a ValueOrExpression[T] field it will also get converted to an expression in the validator.

I understand the str output field type is the legacy form of defining the expression?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes str is legacy support

Copy link
Collaborator Author

@benflexcompute benflexcompute May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can use expression but how do we tell expression what is the target unit? How about we do:

class SolverVariable(Variable):
    ...
    unit_system_name: str = pd.Field("SI")
    
    # def in_SI(self) -> SolverVariable:
    def in_SI(self) -> Expression:
    #    return SolverVariable(unit_system_name = "SI", **(self.model_dump()))
        return Expression(self*conversion_constant)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the conversion rate depends on the entire SimulationParam.... Maybe we can defer the value substitution of conversion_constant?

andrzej-krupka
andrzej-krupka reviewed May 6, 2025
View reviewed changes
andrzej-krupka
andrzej-krupka reviewed May 6, 2025
View reviewed changes
flow360/component/simulation/utils.py
@@ -61,7 +61,7 @@ def is_instance_of_type_in_union(obj, typ) -> bool:

class UnknownFloat(float):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh - this can probably be removed. It was being used by solver variables before but has been deprecated since then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@benflexcompute benflexcompute changed the base branch from expressions to andrzej/expression-solver-translation May 7, 2025 18:25
@andrzej-krupka andrzej-krupka force-pushed the andrzej/expression-solver-translation branch from b7fbe78 to 93518e7 Compare May 8, 2025 14:37
Base automatically changed from andrzej/expression-solver-translation to andrzej/solver-variable-namespace-change May 8, 2025 14:37
@andrzej-krupka andrzej-krupka force-pushed the andrzej/solver-variable-namespace-change branch from 3cc730d to bb44ad2 Compare May 8, 2025 16:46
Base automatically changed from andrzej/solver-variable-namespace-change to expressions May 8, 2025 17:11
@benflexcompute benflexcompute mentioned this pull request May 8, 2025
Merged
@benflexcompute
Copy link
Collaborator Author

Superseded by #1012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrzej-krupka andrzej-krupka andrzej-krupka left review comments

@maciej-flexcompute maciej-flexcompute Awaiting requested review from maciej-flexcompute

@angranl-flex angranl-flex Awaiting requested review from angranl-flex

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benflexcompute @andrzej-krupka